Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix NodePath::slice() incorrect behavior for subname indexing #100954

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Fix NodePath::slice() incorrect behavior for subname indexing #100954

merged 1 commit into from
Jan 10, 2025

Conversation

Bromeon
Copy link
Contributor

@Bromeon Bromeon commented Dec 30, 2024

Following GDScript code:

var path = NodePath("Parent/Child:prop:subprop")
print("slice(0, 1): ", path.slice(0, 1))
print("slice(1, 2): ", path.slice(1, 2))
print("slice(2, 3): ", path.slice(2, 3))
print("slice(3, 4): ", path.slice(3, 4))
print("slice(4, 5): ", path.slice(4, 5))
print("slice(-1):   ", path.slice(-1))
print("slice(-2):   ", path.slice(-2))
print("slice(-3):   ", path.slice(-3))
print("slice(-4):   ", path.slice(-4))

Behavior on master:

slice(0, 1): Parent
slice(1, 2): Child
slice(2, 3): :prop
slice(3, 4): :prop:subprop     <---
slice(4, 5): :subprop          <---

slice(-1):   :prop:subprop     <---
slice(-2):   :prop:subprop
slice(-3):   Child:prop:subprop
slice(-4):   Parent/Child:prop:subprop

I believe the 3 marked results are not intended.
Behavior after this PR:

slice(0, 1): Parent
slice(1, 2): Child
slice(2, 3): :prop
slice(3, 4): :subprop
slice(4, 5): 

slice(-1):   :subprop
slice(-2):   :prop:subprop
slice(-3):   Child:prop:subprop
slice(-4):   Parent/Child:prop:subprop

I updated the unit tests to include 2 subnames, which reveals the problem.

Previously this test existed:

"Slicing on the length of the path should return the last entry."

But this is unintuitive, the last valid index in arrays is always size-1, not size. At least it's not clear to me why one would want this behavior, and it makes iterating through slices non-continuous. So I changed the test, but let me know if there was a reason for this.

@Bromeon
Fix NodePath::slice() incorrect behavior for subname indexing
df66ea7 
Adjust slice boundaries in `NodePath` logic to correctly handle subnames.
Update test cases to reflect these changes.
@Bromeon Bromeon requested review from a team as code owners December 30, 2024 20:49
@Bromeon Bromeon mentioned this pull request Dec 30, 2024
Merged
7 tasks
@Mickeon
Copy link
Contributor

Mickeon commented Dec 31, 2024

This was added in #81822 so it may be nice to hear @nlupugla 's opinion as well.

@nlupugla
Copy link
Contributor

Thanks for finding and fixing this! I haven't had a chance to carefully ponder your changes yet, but as far as the reasoning goes, my intention was for the nodepath slice method to have similar indexing behavior to arrays. So I agree with you that slicing on the last index should actually return empty and the last - 1 should return the last part of the path. Your output looks much more reasonable after your fix :)

@Mickeon Mickeon added this to the 4.4 milestone Dec 31, 2024
Ivorforce
Ivorforce approved these changes Jan 10, 2025
View reviewed changes
Copy link
Contributor

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New behavior makes more sense to me.

@akien-mga
Copy link
Member

akien-mga commented Jan 10, 2025 •
edited
Loading

Might be worth reviewing uses of slice() in the codebase to see if any expect the previous behavior (notably slice(-1) I guess).

Edit: Had a quick look, it doesn't seem to be used a lot, and the uses I found seemed ok.

@akien-mga akien-mga merged commit ad74e33 into godotengine:master Jan 10, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks!

@Bromeon Bromeon deleted the bugfix/nodepath-slice branch January 11, 2025 08:12
@Bromeon Bromeon mentioned this pull request Jan 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@akien-mga akien-mga akien-mga approved these changes

@Ivorforce Ivorforce Ivorforce approved these changes

Assignees
No one assigned
Labels
breaks compat bug topic:core
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@Bromeon @Mickeon @nlupugla @akien-mga @Ivorforce